Revive drawing capabilities (new and improved!)#261
Conversation
| lassign $center cx cy | ||
| set points [list] | ||
| for {set i 0} {$i < $sides} {incr i} { | ||
| set theta [expr {$radians + $i * 2.0 * 3.141592653589793 / $sides - 1.5707963267948966}] |
There was a problem hiding this comment.
I'm slightly surprised that there's no PI (or TAU) constants. Only slightly surprised though.
There was a problem hiding this comment.
Oh yeah, I should definitely just expose PI and Tau globals
|
I plan to test this out on my system today. |
ppkn
left a comment
There was a problem hiding this comment.
This looks good. The drawSpaceLib api feels a little clunky always having to pass poselib and display. I wonder if there is a cleaner way we could do that in the future.
I wrote a small way to preview all of the drawing demos every 2 seconds so I could quickly preview them. It didn't work for shapes.folk though so I had to check that out separately.
When the clock time is /t/ {
set demos {arc circle curve dashed-line fill image line shapes space text}
set i $(int($t)/2 % [llength $demos])
set demo [lindex $demos $i]
Wish $this runs demo code from builtin-programs/draw/${demo}.folk
}
Wish $this is outlined white23d2434 to
e5e18de
Compare
7056a3e to
2282866
Compare
|
Finally going to merge this, it's looking very stable on folk-convivial: folk_shapes_may_29_2026.mp4(Will address bringing back connections.folk and resolving the issues in terminal/terminal-ui.folk in future PRs) |
|
Can you hold off on merging until I can review? Way too many deep changes here for me to be comfortable landing it
…On Fri, May 29, 2026, at 7:41 PM, Andrés Cuervo wrote:
*cwervo* left a comment (FolkComputer/folk#261) <#261 (comment)>
Finally going to merge this, it's looking very stable on folk-convivial:
https://github.com/user-attachments/assets/0cbe158b-4208-4547-999d-ca80ee7ec8ac
(Will address bringing back connections.folk and resolving the issues in terminal/terminal-ui.folk in future PRs)
—
Reply to this email directly, view it on GitHub <#261?email_source=notifications&email_token=AAAXUWKKRDTF5Z6CJLYYEPT45INY3A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINJYGA3DQMJXGY3KM4TFMFZW63VQOJSXM2LFO5PXEZLROVSXG5DFMSSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-4580681766>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAXUWNUICAOHPQXBHRYCU345INY3AVCNFSM6AAAAACYT3FNYWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DKOBQGY4DCNZWGY>.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS <https://github.com/notifications/mobile/ios/AAAXUWIRV363ICFY2SC4PXD45INY3A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINJYGA3DQMJXGY3KM4TFMFZW63VQOJSXM2LFO5PXEZLROVSXG5DFMSSWK5TFNZ2KUZTPN52GK4S7NFXXG> and Android <https://github.com/notifications/mobile/android/AAAXUWKTQBK5J7ZV4MSSUA345INY3A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINJYGA3DQMJXGY3KM4TFMFZW63VQOJSXM2LFO5PXEZLROVSXG5DFMSSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>. Download it today!
You are receiving this because your review was requested.Message ID: ***@***.***>
|
|
@osnr yes, of course! Happy to wait for your review — thanks! |
| layer $layer | ||
| } | ||
|
|
||
| Hold! -on builtin-programs/draw/apriltags.folk -key draw-apriltags-demo-code \ |
There was a problem hiding this comment.
You should just do Claim $this has demo code ... here -- you definitely shouldn't be using Hold! or spelling out the file path.
(in fact, I don't think you should need to use Hold! at all in this entire PR, since it's all purely functional drawing code?)
There was a problem hiding this comment.
(and this comment applies to all the demo code in the PR, none of them should use Hold!)
| width $outlineWidth color $color layer $layer | ||
| } | ||
|
|
||
| When /thing/ has resolved geometry /geom/ &\ |
There was a problem hiding this comment.
This stuff is really ugly:
I would rather standardize on one variant (even if we have to break compatibility) and/or add some syntax sugar (maybe so one handler can deal with both with and non-with variants?) than have all of this stuff all over the codebase -- it's very low-information-density and hard to read.
| // mostly for debugging: | ||
| char* description; | ||
|
|
||
| // Image-wish cache metadata. Cached textures stay resident after the |
| } | ||
|
|
||
| When /someone/ wishes /program/ runs demo code from /filename/ { | ||
| Hold! -key "active-demo-$program" Claim $program running demo code from $filename |
There was a problem hiding this comment.
shouldn't need to use Hold! ; also not a proper English sentence
… early return on 3-point/4-point quads
There was a problem hiding this comment.
Will remove this from this PR tomorrow, accidentally mixed unrelated work back into this branch, apologies!
| return vec4(0.0); | ||
| }]] | ||
|
|
||
| When the color map is /colorMap/ &\ |
| set options [dict merge $options [dict get $result options]] | ||
| } | ||
| set options [dict create x $x y $y scale $defaultScale] | ||
| # FIXME: support per-label options; right now, this just |
There was a problem hiding this comment.
We probably still need this FIXME comment?
| When the color map is /colorMap/ &\ | ||
| /p/ has canvas /id/ with /...wiOptions/ &\ | ||
| /p/ has canvas projection /surfaceToClip/ &\ | ||
| the collected results for [list /someone/ wishes to draw a circle onto /canvas/ with /...options/] are /results/ { |
There was a problem hiding this comment.
why is this a collect and not just a When?
|
|
||
| Wish to draw a polygon onto $page with points $points {*}$fillOpts | ||
| } | ||
|
|
There was a problem hiding this comment.
the rest of this file is weird / repetitive
|
|
||
| set im [{*}$loadImage "[pwd]/vendor/fonts/$name.png"] | ||
| Wish the GPU loads image $im as texture | ||
| Wish the GPU loads image $im as texture with pinned true |
There was a problem hiding this comment.
Vestigial, yep, removed
|
@osnr - a bunch of changes made it through from erroneously merging a previous version of this branch (the source of many of these weird formatting / confusing no-op edits / unnecessary collects). Will go through again this afternoon and fix again, sorry for the mess, I should have waited to re-request review until today. |
| set ::PI 3.142 | ||
| set ::TAU 6.283 | ||
|
|
||
| proc drawTruthy {value} { |
There was a problem hiding this comment.
I think we should probably get rid of this function and just fix the shader value coercion to convert all of these to 1?
| set maxLength $lineLength | ||
| } | ||
| } | ||
| if {$maxLength == 0} { |
There was a problem hiding this comment.
shouldn't we just return early here? also removed the comment for the other set scale
| Wish $p draws a $shape with {*}$options | ||
| } | ||
|
|
||
| When /someone/ wishes /p/ draws a rect with width /width/ height /height/ { |
There was a problem hiding this comment.
mentioned in prev comment but we shouldn't need most of the variants in this file
bc81cb0 to
6a50807
Compare
| tailcall "$drawLib $drawLibCmd" {*}$args | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Does this code do anything / is it actually called? It should already be present in pipelines.folk
There was a problem hiding this comment.
Nope, not called anywhere, removed! Annoyingly, I think I kept recovering old branch state and accidentally committing it.
6a50807 to
9c1edde
Compare
osnr
left a comment
There was a problem hiding this comment.
I really like the flow you can do now where you can incrementally do:
Wish $this draws a circle
Wish $this draws a circle with x 2cm y 2cm
Wish $this draws a circle with x 2cm y 2cm color blue
and have it work at each step.
I still think there are too many helper functions, and they can be difficult to read, but there're no serious blockers here, so I think we should merge it.
One thing I think we should fix quickly in follow-up somehow is that errors (in particular, missing units, which is really easy to do) don't bubble up to the caller program, so you have to go to the web page and look at shapes.folk log specifically (which is not at all obvious, so it feels like a silent draw failure). That might be a system-level fix instead of draw code fix.
|
Yeah, I'm happy with the incremental flow! Alright, good points — will merge now and queue up a fix for the silent erroring and another for cleaning up the helper functions. |
This PR is a little large because it touches a bunch of drawing files. We get back a suite of drawing capabilities. This PR also has an unrelated change to running
make remotewhen working out of a git worktree, happy to splice this into another PR.Some examples of running the demo code via
Wish $this runs demo code from builtin-programs/title.folk(which is a new wish added in this PR):